Require authentication for GraphQL WebSocket upgrade (unomi-3.0.x) - #844
Open
sergehuber wants to merge 3 commits into
Open
Require authentication for GraphQL WebSocket upgrade (unomi-3.0.x)#844sergehuber wants to merge 3 commits into
sergehuber wants to merge 3 commits into
Conversation
2 tasks
Port of the master fix (#843) to the 3.0.x line, re-cut from current unomi-3.0.x. A handshake that carries an Authorization header is authenticated before the upgrade is accepted, and a foreign-origin handshake is refused, since a WebSocket handshake is not subject to the same-origin policy. A handshake that carries no credential - which is all a browser can send - is upgraded but the resulting socket does nothing until it authenticates through the connection_init payload; every other message is refused and closes the socket, and a scheduled close ends any socket that has not authenticated within its deadline. The shipped GraphQL UI passes the Headers-tab Authorization to the WebSocket client as connectionParams so both transports use the same credential. Differences from master, because 3.0.x has no tenancy or security context: whether a socket is authenticated is a plain flag set from the remote user the validator records on a successful handshake login, and there is no execution context to bind around event delivery. The deadline scheduler is shut down from the servlet's destroy(), as the creator object has no Jetty lifecycle of its own. Close frames now carry valid codes (1000/1008) instead of 0, and the credential payload is not logged. Covered by integration tests only, as this line does not carry a unit-test stack for the GraphQL module. GraphQLServletSecurityIT is now registered in AllITs; it was never run on this line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
asf-gitbox-commits
force-pushed
the
backport/graphql-websocket-auth-3.0.x
branch
from
September 4, 2026 10:13
b9f6d6f to
6d55e73
Compare
JUnit resolves the types in a test class's method signatures when it scans the class, which happens at probe start, before the test's setup has waited for the container to finish provisioning. A helper that took a websocket-client type as a parameter therefore made every test in the class fail to load whenever the class ran early, as it does when selected on its own. Build the upgrade request inside the helper instead, so the client bundle is only needed once the tests actually run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Browser-authenticated sockets disable normal idle limits, and the new Basic authentication path has standards-compliance issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Backports authenticated GraphQL WebSocket handling to the 3.0.x branch.
Changes:
- Authenticates header-bearing upgrades and rejects foreign origins.
- Requires browser sockets to authenticate through
connection_init. - Forwards GraphiQL credentials and expands integration coverage.
File summaries
| File | Description |
|---|---|
init-with-credentials.json |
Adds valid initialization fixture. |
init-bad-credentials.json |
Adds invalid initialization fixture. |
GraphQLWebSocketIT.java |
Tests authentication and origin rejection. |
AllITs.java |
Registers security integration tests. |
index.jsx |
Forwards authorization through WebSocket parameters. |
SubscriptionWebSocketFactory.java |
Creates gated sockets and manages deadlines. |
SubscriptionWebSocket.java |
Enforces authentication before operations. |
GraphQLServlet.java |
Secures WebSocket upgrades. |
GraphQLServletSecurityValidator.java |
Adds WebSocket Basic authentication. |
Review details
- Files reviewed: 7/9 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (isAuthenticatedUser(req)) { | ||
| return true; | ||
| } | ||
| res.sendError(HttpServletResponse.SC_UNAUTHORIZED); |
| * @param req the originating request, or {@code null} when the credential did not arrive on one | ||
| */ | ||
| private boolean authenticateBasic(String authHeader, HttpServletRequest req) { | ||
| if (authHeader == null || !authHeader.startsWith("Basic ")) { |
| final Session session = getSession(); | ||
| if (session != null) { | ||
| // Authenticated: drop the short unauthenticated deadline. | ||
| session.setIdleTimeout(0); |
…ore idle timeout A WebSocket upgrade whose credential is refused now receives the same WWW-Authenticate challenge as one that carries no credential, as a 401 must. The Basic scheme token is compared case-insensitively, as HTTP authentication schemes are. A socket that authenticates through connection_init now goes back to the idle timeout its session was configured with instead of having the timeout disabled, so both authentication paths share the same idle behaviour. Adds an integration test authenticating with a lowercase scheme on the handshake. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
unomi-3.0.x, re-cut from the current branch head.Authorizationheader is authenticated before the upgrade is accepted, and a handshake that declares a foreign origin is refused.connection_initpayload: any other message closes it, and a socket that has not authenticated within a short deadline is closed.Authorizationto the WebSocket client asconnectionParams, so both transports use the same credential.destroy(). Close frames now carry valid close codes.Test plan
GraphQLWebSocketITextended to 8 cases andGraphQLServletSecurityITregistered inAllITs; both pass locally under Karaf + Elasticsearch 9.1.3b9b38b132(unit + integration)🤖 Generated with Claude Code